-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
since Detekt uses the standard checkstyle output
import com.novoda.staticanalysis.internal.CollectViolationsTask | ||
import groovy.util.slurpersupport.GPathResult | ||
|
||
class CollectDetektViolationsTask extends CollectViolationsTask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted since Detekt uses Checkstyle format and I don't think they will ever change that.
@@ -29,22 +29,10 @@ build: | |||
comments: 1 | |||
|
|||
processors: | |||
active: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed them since the test output will be cleaner without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a couple of (probably dumb) questions
if (detekt.hasProperty('reports')) { | ||
detekt.reports { | ||
xml.enabled = true | ||
xml.destination = new File(project.buildDir, 'reports/detekt/detekt.xml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tasomaniac does it mean we should warn users about this? is it possible that we read this value and then later the destination is amended lazily by Detekt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means that we always set it by default. Detekt's lazy fallback will never be used.
But that's fine because in case users but their destionation
, that will be used anyways.
@@ -11,15 +12,17 @@ import org.gradle.api.Task | |||
class DetektConfigurator implements Configurator { | |||
|
|||
private static final String DETEKT_PLUGIN = 'io.gitlab.arturbosch.detekt' | |||
private static final String LAST_COMPATIBLE_DETEKT_VERSION = '1.0.0.RC9.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean "we support Detekt up to version X"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I keep updating this whenever I fix new problems :) To be honest, the problems so far were all very weird and were crashing earlier. It might be the case that this was never shown. 🤷
But anyways, it is nice to have.
Detekt RC10 recently changes the destionation File to be lazy by default. The way they do it is pretty weird. They only use the Lazy FileProvider API from Gradle when consumed at a very late stage. By default
destination
is always null in configuration time.This causes problems with our integration.
I tried many things to integrate into their new lazy initialization but it does not seem to be possible. They need to fully use the Lazy FileProvider API.
Instead I set default values for xml reports. If user changes them, it is no problem, they are still picked up.
Additionally, added a check to
enabled
being false. If the user manually disable the xml report, we fail with a proper error message.